Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Web UI uses Webdav instead of ajax/* calls #16902

Merged
merged 19 commits into from
Nov 23, 2015
Merged

Web UI uses Webdav instead of ajax/* calls #16902

merged 19 commits into from
Nov 23, 2015

Conversation

PVince81
Copy link
Contributor

Fixes #12353

  • list
  • move
  • rename
  • new file
  • new folder
  • delete
  • download
  • delete legacy ajax code
  • fix jsocclient namespace / extract as separate lib ?
  • merge OC.Files.FileInfo into OCA.Files.FileInfo
  • remove callbacks, only use Promises
  • remove "isPreviewAvailable"
  • isSharedMount ?
  • error cases, forward status
  • older error messages ? (might be missing from Webdav)
  • check missing properties (isPreviewAvailable, etc)
  • build and verify JS docs!
  • unit tests for the new JS classes
  • TODO separately ?
    • displayed download links in webdav format (public and internal)

Tests

  • TEST: file picker (also uses list.php)
  • TEST: public page
  • TEST: favorites list still works + file operations there
  • TEST: sharing lists still work + file operations there
  • TEST: trashbin still works + file operations
  • TEST: storage not available exception
  • TEST: IMPORTANT: test in IE8 !!!
  • TEST: external storage section in sidebar

Operations

Move
  • TEST: move multiple files
  • TEST: move all files
  • TEST: move file where source does not exist
  • TEST: move file where target does not exist
  • TEST: move file where target already exists
  • TEST: move file where source folder has been removed
Rename
  • TEST: simple rename
  • TEST: rename file where source does not exist any more
  • TEST: rename file where target already exists
  • TEST: rename file where source folder has been removed
  • TEST: rename file with invalid chars
Create file
  • TEST: create file in current folder
  • TEST: create file when current folder does not exist any more
  • TEST: create file when file already exists => shows error message
Create folder
  • TEST: create folder
  • TEST: create folder when current folder does not exist any more
  • TEST: create folder when folder already exists
Delete
  • TEST: delete single file
  • TEST: delete multiple files
  • TEST: delete all files
  • TEST: delete file when source folder does not exist any more
  • TEST: delete file that does not exist any more
Download
  • TEST: single file download (webdav URL)
  • TEST: multiple files download (ajax URL for now, zip)
  • TEST: folder download (ajax URL for now, zip)
  • TEST: single file public page download
  • TEST: multiple files public page download
  • TEST: folder download public page download
  • TEST: public page, download whole page (click download on the top right)
Other
  • TEST: timestamps and timezone, to make sure the library's parsing works as intended
  • TEST: favorites appear in the regular file list
  • TEST: share owner visible in incoming shares
  • TEST: various permissions for files/folders
  • TEST: previews and mime icons still appear
  • TEST: single file preview in public page
  • TEST: MOVE/COPY in reverse proxy environments, because the "Destination" header is absolute, we need to be sure it points to the correct URL

@karlitschek
Copy link
Contributor

Wow. Very cool!!

*
* @return {Promise} promise
*/
list: function(path, callback, includeParent) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why add a callback when we're already returning a promise?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I was more thinking of people not familiar with promises.

Also I'm not sure yet whether I want this "lib" to depend on jquery at all (which provides the promises implementation).

I'll add to my TODO list to reconsider this. Thanks.

@rullzer
Copy link
Contributor

rullzer commented Jun 13, 2015

Awesome! Stuff like this makes me sorry I left my laptop at home. ;-)

On June 12, 2015 1:40:52 PM GMT+02:00, Vincent Petry [email protected] wrote:

Fixes #12353

  • list
  • move
  • rename
  • new file
  • new folder
  • delete
  • upload
  • upload with fallback ?
  • download
  • download (use downloadURL ?)
  • multi-download
  • delete legacy code (list.php, etc)
  • file picker (also uses list.php)
  • test: storage not available exception
  • file ids
  • multi-file selection broken?
  • error cases, forward status
  • older error messages ? (might be missing from Webdav)
  • refresh UI with infos like mimetype, etc
  • token/reload logic (make this more global!)
  • merge OC.Files.FileInfo into OCA.Files.FileInfo
  • check missing properties
  • fix jsocclient namespace / extract as separate lib ?
  • leverage etag knowledge and use preconditions ?
  • missing previews: use @rullzer's mimetype list code ?
  • remove "isPreviewAvailable"
  • isSharedMount ?
  • share properties from sharing ?
  • external storage properties ?
  • allow passing extra properties to list() calls (required for
    favorites, etc)
  • favorites
  • webdav download: x-sendfile!!
  • public page list
  • trashbin broken => split FileList into AbstractFileList (base
    class without) and HomeFileList (for "All files" specific code, using
    WebDAV)
  • check other sidebar sections
  • IMPORTANT: test in IE8 !!! might require IE8 shim
  • build and verify JS docs!
  • sort ? (now client side)
  • getstoragestats: make it use root info ?
  • batch multiple webdav into a single request?
  • JS unit tests

You can view, comment on, or merge this pull request online at:

#16902

-- Commit Summary --

  • Add js-webdav-client library
  • Added class to manage files through webdav
  • Added more webdav ops

-- File Changes --

D apps/files/ajax/delete.php (83)
D apps/files/ajax/move.php (60)
D apps/files/ajax/newfile.php (104)
D apps/files/ajax/newfolder.php (100)
D apps/files/ajax/rename.php (57)
M apps/files/js/file-upload.js (44)
M apps/files/js/filelist.js (138)
M apps/files/js/files.js (19)
M apps/files/lib/app.php (88)
M bower.json (5)
M core/js/js.js (8)
A core/js/jsocclient.js (512)
A core/vendor/js-webdav-client/.bower.json (15)
A core/vendor/js-webdav-client/.gitmodules (3)
A core/vendor/js-webdav-client/COPYING.LESSER.txt (165)
A core/vendor/js-webdav-client/LICENSE.txt (674)
A core/vendor/js-webdav-client/scripts/check_dependencies.sh (25)
A core/vendor/js-webdav-client/tests/resources/mock.js (468)
A core/vendor/js-webdav-client/tests/resources/qunit.css (244)
A core/vendor/js-webdav-client/tests/resources/qunit.js (2212)
A core/vendor/js-webdav-client/tests/templates/tests_footer.html (2)
A core/vendor/js-webdav-client/tests/templates/tests_header.html (12)
A core/vendor/js-webdav-client/tests/unittests/0000_exception.js (46)
A core/vendor/js-webdav-client/tests/unittests/0010_codec.js (72)
A core/vendor/js-webdav-client/tests/unittests/0020_property.js (175)
A core/vendor/js-webdav-client/tests/unittests/0030_response.js (153)
A core/vendor/js-webdav-client/tests/unittests/0040_multistatus.js (72)
A core/vendor/js-webdav-client/tests/unittests/0050_client.js (1065)
A
core/vendor/js-webdav-client/tests/unittests/plugins/acl/0000_privilege.js
(104)
A core/vendor/js-webdav-client/tests/unittests/plugins/acl/0010_ace.js
(159)
A core/vendor/js-webdav-client/tests/unittests/plugins/acl/0020_acl.js
(186)
A
core/vendor/js-webdav-client/tests/unittests/plugins/acl/0030_acl_codec.js
(179)
A
core/vendor/js-webdav-client/tests/unittests/plugins/acl/0040_client_acl.js
(223)
A
core/vendor/js-webdav-client/tests/unittests/plugins/acl/current-user-privilege-set_codec.js
(75)
A
core/vendor/js-webdav-client/tests/unittests/plugins/acl/inherited-acl-set_codec.js
(66)
A
core/vendor/js-webdav-client/tests/unittests/plugins/acl/principal-collection-set_codec.js
(66)
A
core/vendor/js-webdav-client/tests/unittests/plugins/webdav_default_codecs/creationdate.js
(53)
A
core/vendor/js-webdav-client/tests/unittests/plugins/webdav_default_codecs/getcontentlength.js
(53)
A
core/vendor/js-webdav-client/tests/unittests/plugins/webdav_default_codecs/getlastmodified.js
(54)
A
core/vendor/js-webdav-client/tests/unittests/plugins/webdav_default_codecs/owner.js
(89)
A
core/vendor/js-webdav-client/tests/unittests/plugins/webdav_default_codecs/resourcetype.js
(72)
M lib/base.php (3)

-- Patch Links --

https://github.com/owncloud/core/pull/16902.patch
https://github.com/owncloud/core/pull/16902.diff


Reply to this email directly or view it on GitHub:
#16902

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@PVince81
Copy link
Contributor Author

@rullzer coding on the beach is probably not a good idea anyway, you don't want sand in your laptop.

@DeepDiver1975
Copy link
Member

@PVince81 rebase please

Can we do this step by step? As in merge now and fix remaining issues in a followup prs?

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 2, 2015

Yeah, I'll try to split them to avoid causing regressions.

@DeepDiver1975
Copy link
Member

Yeah, I'll try to split them to avoid causing regressions.

I guess there is not need to split the existing pr - just bring this to a working state and merge ....

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 2, 2015

Working state is a big word. There is still quite some work needed because PROPFIND doesn't return all the information we used to have with the ajax call so there is a high risk of regression.

By splitting I could remove the "list" operation and at least merge the simpler ones.

I'll see what I can do.

@DeepDiver1975
Copy link
Member

I'll see what I can do.

thx

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 8, 2015

  • BUG: public page broken

I'll move the todos back to the original ticket to be done separately.
Trying to get a working version from what we already have here 😄 (working as in "does not break other things")

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 8, 2015

@icewind1991 ok I think I'll only use promises, no callbacks.
This means the client lib will currently rely on jQuery, but that's fine for now.

@icewind1991
Copy link
Contributor

@icewind1991 ok I think I'll only use promises, no callbacks.
This means the client lib will currently rely on jQuery, but that's fine for now.

I wish we could just use babel and write es6/7, the async/await syntax is soooo nice 😄

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 8, 2015

Yeah, but we'd need an IE8 shim or something. Maybe one day...

Public page now works and also supports rename, delete, move, create folder 😄

  • TODO: auth error cases in reloadCallback (token expired, etc) 4fe1cdd

Added better error handling and failing promises.
It is nice to see that Webdav has specific codes to distinguish "target already exists" or "parent folder does not exist for the resource you wanted to touch", etc. Makes it possible to display customized error messages.

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 9, 2015

  • missing share owner info in response (indicator missing): new Webdav property ?
  • missing external storage icon: could be done as an additional: new Webdav property ? or reuse resource type ?

@DeepDiver1975
Copy link
Member

missing external storage icon: could be done as an additional: new Webdav property ? or reuse resource type ?

our permissions property already contains M - to tell if a folder/file is mounted

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 9, 2015

Right, I could use that. But the old web UI had more detailed info about the mount type.
Also at some point we'll want to use the specific icon for ext storage mounts (WND, etc).

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 9, 2015

Solved the mount types without extra attributes.

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 9, 2015

  • rewire existing unit tests to check if OC.Files.Client is called properly (stub it)
  • write tests for OC.Files.Client and check with sinon's fakeServer that the requests are made properly
    • check encoding/spaces (especially for MOVE destination)
    • check path building (no doubled slashes)
    • test with sample Webdav responses, parsing must work properly
  • make publicpreview work when s2s is disabled (maybe use a special header)

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 9, 2015

@oparoz @rullzer regarding previews: if I remember well there was a decision to simply let them fail with 404 when they don't exist ?
My problem now is that the Webdav response doesn't contain "isPreviewAvailable" any more, so I'd need to indiscriminately request the preview for any file (except folders). Probably at some point we'll also wait to have a OC.Previews namespace with a list of supported previews.

Ideally I'd like to get rid of "isPreviewAvailable" now. It makes the logic quite complex.

@rullzer
Copy link
Contributor

rullzer commented Jul 9, 2015

@PVince81 the OC.Previews namespace sounds like a good approach... makes it also more clean IMO. Just drop the isPreviewAvailable.

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 9, 2015

Ok thanks @rullzer, raised here #17543

@PVince81
Copy link
Contributor Author

Without isPreviewAvailable, we might get a lot of 404 for files that don't have previews. In this case we should look into caching the 404 errors: #17557

Vincent Petry and others added 15 commits November 22, 2015 16:05
This makes it possible to also store custom properties passed through
the data object like tags or shareOwner.
This prevents double slashes that can mess up path comparisons in some
cases.
This used to be done in the ajax download code. Now that single file
downloads are going through Webdav, the token handling needs to be done
here too.
This will make sure the cached JS gets properly updated.
Also, since this is a bigger change it also qualifies for a version
increase :-)
@LukasReschke
Copy link
Member

This looks strange:

10:08:39 There was 1 failure:
10:08:39 
10:08:39 1) Test\Files\Storage\SMB::testStat
10:08:39 Failed asserting that 1448204888 is equal to 1448204886 or is less than 1448204886.
10:08:39 
10:08:39 /var/lib/jenkins/jobs/server-master-linux-externals-smb-windows-ext-ci/workspace/database/sqlite/external/smb-windows/label/master/tests/lib/files/storage/storage.php:295
10:08:39 

@DeepDiver1975
Copy link
Member

Clock on the Windows vm is out if sync. Unrelated from my pov

@MorrisJobke
Copy link
Contributor

@rullzer @nickvergessen @LukasReschke One more review please :) We need to get this in :)

@LukasReschke
Copy link
Member

Works 👍

DeepDiver1975 added a commit that referenced this pull request Nov 23, 2015
Web UI uses Webdav instead of ajax/* calls
@DeepDiver1975 DeepDiver1975 merged commit 79bbda9 into master Nov 23, 2015
@DeepDiver1975 DeepDiver1975 deleted the jsocclient branch November 23, 2015 08:38
@PVince81
Copy link
Contributor Author

Great news for a monday morning 😄
Thanks guys !

@LukasReschke
Copy link
Member

🎉 🚀 💃 🍻

@felixboehm
Copy link
Contributor

You rock!

@jospoortvliet
Copy link

Party time indeed!!! Awesome.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move files app to use WebDAV instead of custom PHP endpoints
10 participants